feat(skill): supports baseDir being treated as a single skill directory#1824
feat(skill): supports baseDir being treated as a single skill directory#1824chcodex wants to merge 5 commits into
Conversation
- If baseDir itself contains SKILL.md, only the skill information of this directory will be returned. - Modify the getAllSkillNames method to support root directory skill recognition - Modify the getAllSkills method to support root directory skill loading - Modify loadSkill method to support root directory skill name matching check - Modify the skillExists method to support root directory skill existence judgment - Add unit tests to cover the new root directory skill processing logic - Unit tests verify the correct name, loading and existence of root skills
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
AgentScopeJavaBot
left a comment
AgentScopeJavaBot
left a comment
There was a problem hiding this comment.
🤖 AI Review
This PR adds support for treating baseDir itself as a single skill directory when it contains a SKILL.md file. The changes cover getAllSkillNames, getAllSkills, loadSkill (via findSkillDirectoryByName), and skillExists. Unit tests are included. However, there is a critical consistency defect: findSkillDirectoryByName does not early-return when baseDir has SKILL.md but the name doesn't match — it falls through to scan subdirectories, contradicting getAllSkillNames/getAllSkills which short-circuit in that case. Additionally, deleteSkill could accidentally delete the entire baseDir if the root-level skill name matches.
(inline comments could not be attached — line numbers fell outside PR hunks. See archived report.)
AgentScopeJavaBot
left a comment
AgentScopeJavaBot
left a comment
There was a problem hiding this comment.
🤖 AI Review
This PR adds support for treating baseDir itself as a single skill directory when it contains a SKILL.md file. The changes cover getAllSkillNames, getAllSkills, loadSkill (via findSkillDirectoryByName), and skillExists. Unit tests are included. However, there is a critical consistency defect: findSkillDirectoryByName does not early-return when baseDir has SKILL.md but the name doesn't match — it falls through to scan subdirectories, contradicting getAllSkillNames/getAllSkills which short-circuit in that case. Additionally, deleteSkill could accidentally delete the entire baseDir if the root-level skill name matches.
(inline comments could not be attached — line numbers fell outside PR hunks. See archived report.)
AgentScopeJavaBot
added
enhancement
|
Thanks for the review! Issue 1 — findSkillDirectoryByName consistency (fixed ✅) Issue 2 — deleteSkill might delete entire baseDir |
|
@chickenlj Hello, please help to see if this PR is reasonable? |
2 participants
AgentScope-Java Version
2.0.0-SNAPSHOT
Description
Background
When a skill repository's baseDir itself contains a SKILL.md file, the current implementation does not recognize it as a valid skill directory, requiring users to place SKILL.md in a subdirectory under baseDir.
Changes Made
getAllSkillNames()to support root directory skill recognitiongetAllSkills()to support root directory skill loadingloadSkill()to support root directory skill name matching checkskillExists()to support root directory skill existence judgmentHow to Test
getAllSkills()and verify the root skill is returnedskillExists()with the root skill name and verify it returns trueloadSkill()with the root skill name and verify it loads successfullyChecklist
mvn spotless:applymvn test)